feat: scoped accounts#161
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR migrates the interpreter balance representation from a map to a row-based ChangesBalances model, typed value semantics, and scoped builtin
Sequence Diagram(s)sequenceDiagram
participant Parser
participant Analyzer
participant Interpreter
participant CachedBalances
participant Store
Parser->>Analyzer: scoped(`@src`, "x")
Analyzer->>Analyzer: checkFnCall validates FnVarOriginScoped registered
Interpreter->>Interpreter: evaluate scoped(account, scope)
Interpreter->>Interpreter: expectAccount validates `@src`
Interpreter->>Interpreter: expectString extracts "x"
Interpreter->>Interpreter: validateScope checks ^[a-z0-9_]*$
Interpreter-->>Parser: AccountAddress{Name: "src", Scope: "x"}
Parser->>Interpreter: send via scoped source
Interpreter->>CachedBalances: fetchBalance(src/x, asset, color)
CachedBalances-->>Interpreter: *big.Int balance
Interpreter->>Store: query balances for src/x
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ascandone
force-pushed
the
feat/scoped-accounts
branch
from
3613cf9 to
bcba9a9
Compare
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The scoped function is gated by the wrong feature flag in static analysis, and scopes containing digits are accepted by the new validator but rejected by final account validation. These issues can make valid scoped-account scripts fail.
| VersionConstraints: []VersionClause{ | ||
| { | ||
| Version: parser.NewVersionInterpreter(0, 0, 25), | ||
| FeatureFlag: flags.ExperimentalGetAmountFunctionFeatureFlag, |
There was a problem hiding this comment.
🟠 [major] Use the scoped feature flag for scoped()
For versioned scripts using scoped() with // @feature_flag experimental-scoped-function, static analysis still reports that experimental-get-amount-function is required because the version constraint is wired to the wrong flag. This makes valid scoped scripts fail analysis unless they enable an unrelated flag, while runtime still checks for the scoped flag.
| const accountSegmentRegex = "[a-zA-Z0-9_-]+" | ||
|
|
||
| var accountNameRegex = regexp.MustCompile("^" + accountSegmentRegex + "(:" + accountSegmentRegex + ")*$") | ||
| var accountNameRegex = regexp.MustCompile("^@?" + accountSegmentRegex + "(:" + accountSegmentRegex + ")*(?:/[a-z_]+)?$") |
There was a problem hiding this comment.
🟠 [major] Accept the same scope syntax in account validation
When the scope contains a digit, e.g. scoped(@src, "x1"), validateScope accepts it and returns src/x1, but this account regex only permits [a-z_] after /. The generated posting is then rejected by checkPostingInvariants as an internal error, so valid scoped transfers fail; the account regex should match the scope validator.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The patch introduces mismatched feature-flag validation and inconsistent account/scope validation that can either reject valid scoped accounts later as internal errors or allow invalid @-prefixed account values through.
| VersionConstraints: []VersionClause{ | ||
| { | ||
| Version: parser.NewVersionInterpreter(0, 0, 25), | ||
| FeatureFlag: flags.ExperimentalGetAmountFunctionFeatureFlag, |
There was a problem hiding this comment.
🟠 [major] Use the scoped feature flag in analysis
For versioned scripts that enable experimental-scoped-function, the checker still reports experimental-get-amount-function as the missing/required flag for scoped(...). This also lets analysis pass when only the get-amount flag is enabled, even though the interpreter later rejects the call with experimental-scoped-function, so the static and runtime feature gates disagree.
| const accountSegmentRegex = "[a-zA-Z0-9_-]+" | ||
|
|
||
| var accountNameRegex = regexp.MustCompile("^" + accountSegmentRegex + "(:" + accountSegmentRegex + ")*$") | ||
| var accountNameRegex = regexp.MustCompile("^@?" + accountSegmentRegex + "(:" + accountSegmentRegex + ")*(?:/[a-z_]+)?$") |
There was a problem hiding this comment.
🟠 [major] Reject @-prefixed account inputs
When an account is provided through a typed variable, parseVar passes the raw value to NewAccountAddress, and checkPostingInvariants also relies on this regex. Allowing the leading @ means raw variable values like @src now validate and can be emitted in postings, even though @ is DSL syntax and not part of the ledger account address.
| const accountSegmentRegex = "[a-zA-Z0-9_-]+" | ||
|
|
||
| var accountNameRegex = regexp.MustCompile("^" + accountSegmentRegex + "(:" + accountSegmentRegex + ")*$") | ||
| var accountNameRegex = regexp.MustCompile("^@?" + accountSegmentRegex + "(:" + accountSegmentRegex + ")*(?:/[a-z_]+)?$") |
There was a problem hiding this comment.
🟠 [major] Allow digit scopes in posting validation
The new validateScope accepts scopes containing digits, so scoped(@src, "x1") succeeds, but this account regex only allows [a-z_]+ after /. When that scoped account is used in a posting, checkPostingInvariants rejects it as an internal error, so scopes that the new function explicitly accepts fail later in execution.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The patch contains a build-blocking compile error in the new scoped function. Even after that, scoped accounts are not wired consistently through feature gating, JSON/spec decoding, and account validation.
Findings outside the diff
🟠 [major] Preserve scoped fields in expected postings — internal/interpreter/interpreter.go:26
When a specs file uses the new sourceScope or destinationScope fields for expect.postings, those fields are ignored because Posting has no corresponding JSON fields or custom unmarshal logic. The expected posting remains src -> dest, while the interpreter emits scoped account names such as src/x -> dest/y, so scoped posting assertions cannot pass using the documented schema.
| Docs: "returns the scoped version of that account. Empty string means no scope. Overwrites the previous scope", | ||
| VersionConstraints: []VersionClause{ | ||
| { | ||
| Version: parser.NewVersionInterpreter(0, 0, 25), |
There was a problem hiding this comment.
🔴 [blocker] Require the scoped feature flag for scoped()
When a script enables only experimental-scoped-function, analysis still reports the get-amount feature as missing because this builtin is wired to ExperimentalGetAmountFunctionFeatureFlag. The new scoped fixture declares the scoped flag but not get-amount, so it will be rejected before runtime; this should reference ExperimentalScopedFunction instead.
| } | ||
| } | ||
| return cloned | ||
| type BalanceRow struct { |
There was a problem hiding this comment.
🔴 [blocker] Decode balance scopes before storing rows
When specs or input JSON uses the newly documented scope property, encoding/json drops it because BalanceRow has no matching field, so StaticStore stores the balance under src while scoped(@src, "x") queries src/x. Scoped balances from the schema and the new fixture therefore appear as zero/missing funds unless the scope is represented and folded into the account key.
| @@ -248,7 +181,7 @@ func (s *programState) parseVars(varDeclrs []parser.VarDeclaration, rawVars map[ | |||
|
|
|||
| const accountSegmentRegex = "[a-zA-Z0-9_-]+" | |||
There was a problem hiding this comment.
🟠 [major] Allow digits in scoped account validation
For scopes containing digits, validateScope and the JSON schemas accept values like x1, but checkAccountName rejects the resulting account because the scoped suffix only allows [a-z_]. A script using scoped(@src, "x1") can pass scope validation and then fail the final posting invariant as an internal invalid account.
| return nil, err | ||
| } | ||
|
|
||
| if !validateScope(*scope) { |
There was a problem hiding this comment.
🟡 [minor] Stop dereferencing parsed scoped arguments
With the new parseArg signature, acc and scope are values (AccountAddress/String), not pointers, so *scope and *acc do not compile. This blocks the package from building as soon as this file is compiled; use the values directly or convert them to string where needed.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/specs_format/index.go (1)
87-107:⚠️ Potential issue | 🟡 MinorCheck function has an implicit precondition: specs must be pre-validated, but this is not documented.
While the production code path in
runRawSpec(runner.go:160) does guardCheckwithvalidateSpecs, theCheckfunction is exported and lacks documentation of this precondition. Tests intentionally callCheckdirectly without validation—which is fine for unit testing—but any external caller or future code path that directly invokesCheckcould bypass the guard.Consider either:
- Adding a docstring to
Checkdocumenting the precondition thatvalidateSpecsmust be called first- Making
Checkinternal (unexported) to enforce the contract at the type level- Moving validation into
Checkitself to ensure it always holds🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specs_format/index.go` around lines 87 - 107, The exported Check function has an implicit precondition that the specs parameter must be pre-validated by calling validateSpecs first, but this requirement is not documented. Add a docstring to the Check function that clearly documents this precondition, explaining that callers must ensure specs have been validated before invoking Check. This will make the contract explicit and prevent external callers or future code paths from accidentally bypassing the validation guard.
♻️ Duplicate comments (2)
internal/analysis/check.go (1)
127-131:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the scoped feature flag for
scoped()version gating.Line 130 still references
flags.ExperimentalGetAmountFunctionFeatureFlagforFnVarOriginScoped. This makes static analysis require the wrong flag forscoped()and can diverge from interpreter behavior. Switch it toflags.ExperimentalScopedFunction.Proposed fix
FnVarOriginScoped: VarOriginFnCallResolution{ Params: []string{TypeAccount, TypeString}, Return: TypeAccount, Docs: "returns the scoped version of that account. Empty string means no scope. Overwrites the previous scope", VersionConstraints: []VersionClause{ { Version: parser.NewVersionInterpreter(0, 0, 25), - FeatureFlag: flags.ExperimentalGetAmountFunctionFeatureFlag, + FeatureFlag: flags.ExperimentalScopedFunction, }, }, },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/analysis/check.go` around lines 127 - 131, The VersionClause for FnVarOriginScoped (the scoped() function) in the check.go file is using the incorrect feature flag. Replace flags.ExperimentalGetAmountFunctionFeatureFlag with flags.ExperimentalScopedFunction in the FeatureFlag field of the VersionClause that has Version 0.0.25. This ensures that static analysis uses the correct scoped feature flag for version gating and aligns with the interpreter behavior.internal/interpreter/interpreter.go (1)
184-184:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep canonical account validation free of the DSL
@sigil.Line 184 now accepts an optional leading
@. This allows raw variable values like@srcto validate as account addresses and propagate into postings/store-facing data, which breaks canonical account contract boundaries.Suggested fix
-var accountNameRegex = regexp.MustCompile("^@?" + accountSegmentRegex + "(:" + accountSegmentRegex + ")*(?:/[a-z_]+)?$") +var accountNameRegex = regexp.MustCompile("^" + accountSegmentRegex + "(:" + accountSegmentRegex + ")*(?:/[a-z_]+)?$")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/interpreter/interpreter.go` at line 184, The accountNameRegex variable at line 184 includes an optional leading @ character (^@?) which allows DSL-specific variable values like `@src` to be validated as canonical account addresses, violating the account contract boundary. Remove the @? optional matching from the beginning of the regex pattern so it only validates canonical account names without the DSL @ sigil prefix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/interpreter/balances.go`:
- Around line 83-88: The CompareBalances function can incorrectly return true
for non-equal multisets when duplicate (account, asset, color) keys exist,
because CompareBalancesIncluding only resolves the first match of each key. Add
duplicate key validation in the CompareBalances function to check both input
Balances for duplicate keys and return false immediately if any duplicates are
found, ensuring that only well-formed (non-duplicate) Balances can pass the
equality check.
In `@internal/interpreter/evaluate_expr.go`:
- Around line 154-158: The code unconditionally calls colorExpr.GetRange() when
constructing the InvalidColor error return at line 157, but colorExpr can be nil
according to the evaluateOptExprAs contract. Add a nil check for colorExpr
before dereferencing it; if colorExpr is nil, either omit the Range field from
the InvalidColor struct or provide a zero/default Range value instead of calling
GetRange().
In `@internal/interpreter/function_exprs.go`:
- Around line 181-185: Remove the invalid pointer dereferences in the scoped()
function. The parameters scope and acc are value types (string type aliases),
not pointers, so they should not be dereferenced with the * operator. Remove the
* prefix from all occurrences: the *scope in the validateScope call, the *scope
in the InvalidScope struct literal, and both *acc and *scope in the appendScope
call.
In `@internal/interpreter/internal_balances.go`:
- Around line 71-82: In the Set method of InternalBalances, the code currently
stores caller-owned *big.Int pointers directly (at both the assignment on line
74 and the append on line 81), which allows external mutation of cached state
and permits nil pointers to enter the runtime balance state. Copy the amount
value instead of storing the pointer directly: create a new big.Int and use the
Set method to copy the value (e.g., new(big.Int).Set(amount)) at both locations
where Amount is assigned, and ensure nil amounts are handled appropriately by
normalizing them to zero or a safe default big.Int value.
In `@internal/interpreter/store.go`:
- Around line 47-52: Add nil checks before dereferencing row.Amount in the
BalanceRow initialization within the store materialization code. When
constructing BalanceRow objects and assigning Amount using
new(big.Int).Set(row.Amount), first check if row.Amount is nil to prevent panics
during query execution. Apply this same nil-checking pattern to all locations
where row.Amount is dereferenced through the Set method call, ensuring the code
gracefully handles null amount values.
In `@internal/mcp_impl/handlers.go`:
- Around line 69-72: The error handling for BindArguments is inconsistent with
other error handling in this handler. Currently, when BindArguments fails, the
code returns (nil, err) which propagates it as a server-level error. Refactor
this to wrap the error in mcp.NewToolResultError(...) and return
(mcp.NewToolResultError(...), nil) instead, matching the pattern used for other
errors in the same handler such as RequireString failures and interpreter
errors. This ensures all tool-level errors are handled consistently as
structured tool result errors rather than server-level errors.
---
Outside diff comments:
In `@internal/specs_format/index.go`:
- Around line 87-107: The exported Check function has an implicit precondition
that the specs parameter must be pre-validated by calling validateSpecs first,
but this requirement is not documented. Add a docstring to the Check function
that clearly documents this precondition, explaining that callers must ensure
specs have been validated before invoking Check. This will make the contract
explicit and prevent external callers or future code paths from accidentally
bypassing the validation guard.
---
Duplicate comments:
In `@internal/analysis/check.go`:
- Around line 127-131: The VersionClause for FnVarOriginScoped (the scoped()
function) in the check.go file is using the incorrect feature flag. Replace
flags.ExperimentalGetAmountFunctionFeatureFlag with
flags.ExperimentalScopedFunction in the FeatureFlag field of the VersionClause
that has Version 0.0.25. This ensures that static analysis uses the correct
scoped feature flag for version gating and aligns with the interpreter behavior.
In `@internal/interpreter/interpreter.go`:
- Line 184: The accountNameRegex variable at line 184 includes an optional
leading @ character (^@?) which allows DSL-specific variable values like `@src` to
be validated as canonical account addresses, violating the account contract
boundary. Remove the @? optional matching from the beginning of the regex
pattern so it only validates canonical account names without the DSL @ sigil
prefix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce101ddb-95c1-4b5f-a5ba-b512764f07fe
⛔ Files ignored due to path filters (140)
inputs.schema.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/experimental/mixed-source-prefer-single-source.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/experimental/top-up-many.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/experimental/top-up.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/experimental/transfer-example.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/mixed-source-dest-allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/send-max.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/top-up-max.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/allocate-dont-take-too-much.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/allocation.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/ask-balance-twice.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/balance-not-found.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/balance-simple.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/bigint-literal.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/capped-when-less-than-needed.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/capped-when-more-than-balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/cascading-sources.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/do-not-exceed-overdraft-on-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/do-not-exceed-overdraft-when-double-spending.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/do-not-exceed-overdraft.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/dynamic-allocation.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/empty-postings.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/account-interpolation/account-interp.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-inorder-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-inorder.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-restrict-balance-when-missing-funds.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-restrict-balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-restriction-in-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-send-overdrat.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-send.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-with-asset-precision.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/empty-color.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/no-double-spending-in-colored-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/no-double-spending-in-colored-send.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/no-solution.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-all-allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-kept.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-with-oneof.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/update-swap-account-balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/get-amount-function/get-amount-function.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/get-asset-function/get-asset-function.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/mid-script-function-call/expr-in-var-origin.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/mid-script-function-call/midscript-balance-after-decrease.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/mid-script-function-call/midscript-balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/oneof/oneof-all-failing.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/oneof/oneof-in-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/oneof/oneof-in-source-send-first-branch.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/oneof/oneof-in-source.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/oneof/oneof-singleton.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/oneof/update-balances-with-oneof.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/overdraft-function/overdraft-function-use-case-remove-debt.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/overdraft-function/overdraft-function-when-negative.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/overdraft-function/overdraft-function-when-positive.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/overdraft-function/overdraft-function-when-zero.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/overdraft-function/reach-zero.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/scoped-function/simple.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/feature-flag-syntax.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/floating-perc.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/floating-perc2.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/inoder-destination.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/insufficient-funds.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/kept-in-send-all-inorder.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/kept-with-balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/many-kept-dest.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/many-max-dest.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/max-with-unbounded-overdraft.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/metadata.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/minus-infix-monetary.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/minus-infix-number.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/minus-prefix-number.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/neg-max-dest.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/negative-max-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/negative-max.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/nested-remaining-complex.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/ovedrafts-playground-example.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/overdraft-in-send-all-when-noop.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/overdraft-in-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/overdraft-not-enough-funds.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/overdraft-when-enough-funds.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/overdraft-when-negative-balance-in-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/overdraft-when-negative-balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/overdraft-when-negative-ovedraft-in-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/overdraft-when-not-enough-funds.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/remaining-kept-in-send-all-inorder.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/remaining-none-in-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/save/save-from-account__multi-postings.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/save/save-from-account__save-a-different-asset.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/save/save-from-account__save-all-negative-balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/save/save-from-account__save-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/save/save-from-account__save-causes-failure.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/save/save-from-account__save-more-than-balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/save/save-from-account__simple.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/save/save-from-account__with-asset-var.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/save/save-from-account__with-monetary-var.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-all-destinatio-allot-complex.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-all-destinatio-allot.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-all-many-max-in-dest.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-all-multi.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-all-variable.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-all-when-negative-with-overdraft.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-all-when-negative.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-allt-max-in-dest.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-allt-max-in-src.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-allt-max-when-no-amount.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-when-negative-balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-zero.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/source-allotment-invalid-amt.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/source-allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/source-complex.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/source-overlapping.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/source.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/track-balances-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/track-balances.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/track-balances2.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/track-balances3.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/unbounded-overdraft-when-not-enough-funds.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/update-balances.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/use-balance-twice.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/use-different-assets-with-same-source-account.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/variable-asset.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/variable-balance__1.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/variable-balance__2.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/variable-balance__3.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/variable-balance__4.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/variable-balance__5.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/variables-json.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/variables.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/world-source.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/zero-postings-explicit-allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/zero-postings-explicit-inorder.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/zero-postings.num.specs.jsonis excluded by!**/*.jsoninternal/parser/__snapshots__/parser_test.snapis excluded by!**/*.snap,!**/*.snapinternal/specs_format/__snapshots__/runner_test.snapis excluded by!**/*.snap,!**/*.snapspecs.schema.jsonis excluded by!**/*.json
📒 Files selected for processing (36)
internal/analysis/check.gointernal/cmd/run.gointernal/cmd/test_init.gointernal/cmd/test_init_test.gointernal/flags/flags.gointernal/interpreter/accounts_metadata.gointernal/interpreter/append_scope.gointernal/interpreter/append_scope_test.gointernal/interpreter/args_parser.gointernal/interpreter/args_parser_test.gointernal/interpreter/asset_scaling.gointernal/interpreter/balances.gointernal/interpreter/balances_test.gointernal/interpreter/batch_balances_query.gointernal/interpreter/evaluate_expr.gointernal/interpreter/function_exprs.gointernal/interpreter/function_statements.gointernal/interpreter/infix_overloads.gointernal/interpreter/internal_balances.gointernal/interpreter/internal_balances_test.gointernal/interpreter/interpreter.gointernal/interpreter/interpreter_error.gointernal/interpreter/interpreter_test.gointernal/interpreter/store.gointernal/interpreter/testdata/script-tests/experimental/scoped-function/simple.numinternal/interpreter/value.gointernal/mcp_impl/handlers.gointernal/parser/ast.gointernal/specs_format/index.gointernal/specs_format/parse_test.gointernal/specs_format/run_test.gointernal/specs_format/runner.gointernal/specs_format/runner_test.gointernal/utils/utils.gonumscript.gonumscript_test.go
💤 Files with no reviewable changes (2)
- internal/utils/utils.go
- internal/parser/ast.go
| func CompareBalances(b1 Balances, b2 Balances) bool { | ||
| return utils.Map2Cmp(b1, b2, func(ab1, ab2 *big.Int) bool { | ||
| return ab1.Cmp(ab2) == 0 | ||
| }) | ||
| if len(b1) != len(b2) { | ||
| return false | ||
| } | ||
| return CompareBalancesIncluding(b1, b2) | ||
| } |
There was a problem hiding this comment.
Reject duplicate balance keys before equality comparison.
CompareBalances can return true for non-equal multisets when duplicate (account, asset, color) keys exist, because Line 93 resolves only the first match. Since duplicates are explicitly modeled as malformed, reject them up-front in equality checks.
Suggested fix
func CompareBalances(b1 Balances, b2 Balances) bool {
+ if _, dup := b1.FirstDuplicate(); dup {
+ return false
+ }
+ if _, dup := b2.FirstDuplicate(); dup {
+ return false
+ }
if len(b1) != len(b2) {
return false
}
return CompareBalancesIncluding(b1, b2)
}Also applies to: 91-98
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/interpreter/balances.go` around lines 83 - 88, The CompareBalances
function can incorrectly return true for non-equal multisets when duplicate
(account, asset, color) keys exist, because CompareBalancesIncluding only
resolves the first match of each key. Add duplicate key validation in the
CompareBalances function to check both input Balances for duplicate keys and
return false immediately if any duplicates are found, ensuring that only
well-formed (non-duplicate) Balances can pass the equality check.
| isValidColor := colorRe.Match([]byte(string(color))) | ||
| if !isValidColor { | ||
| return nil, InvalidColor{ | ||
| return "", InvalidColor{ | ||
| Range: colorExpr.GetRange(), | ||
| Color: *color, | ||
| Color: string(color), |
There was a problem hiding this comment.
Guard nil colorExpr before using GetRange().
Line 157 dereferences colorExpr even though evaluateOptExprAs permits colorExpr == nil. If empty color is rejected, this can panic at runtime.
Proposed fix
func (s *programState) evaluateColor(colorExpr parser.ValueExpr) (String, InterpreterError) {
+ if colorExpr == nil {
+ return "", nil
+ }
+
color, err := evaluateOptExprAs(s, colorExpr, expectString)
if err != nil {
return "", err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| isValidColor := colorRe.Match([]byte(string(color))) | |
| if !isValidColor { | |
| return nil, InvalidColor{ | |
| return "", InvalidColor{ | |
| Range: colorExpr.GetRange(), | |
| Color: *color, | |
| Color: string(color), | |
| func (s *programState) evaluateColor(colorExpr parser.ValueExpr) (String, InterpreterError) { | |
| if colorExpr == nil { | |
| return "", nil | |
| } | |
| color, err := evaluateOptExprAs(s, colorExpr, expectString) | |
| if err != nil { | |
| return "", err | |
| } | |
| // ... rest of function continues |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/interpreter/evaluate_expr.go` around lines 154 - 158, The code
unconditionally calls colorExpr.GetRange() when constructing the InvalidColor
error return at line 157, but colorExpr can be nil according to the
evaluateOptExprAs contract. Add a nil check for colorExpr before dereferencing
it; if colorExpr is nil, either omit the Range field from the InvalidColor
struct or provide a zero/default Range value instead of calling GetRange().
| func (b InternalBalances) Set(account string, asset string, color string, amount *big.Int) { | ||
| for i := range b[account] { | ||
| if b[account][i].Asset == asset && b[account][i].Color == color { | ||
| b[account][i].Amount = amount | ||
| return | ||
| } | ||
| } | ||
| b[account] = append(b[account], AccountBalance{ | ||
| Asset: asset, | ||
| Color: color, | ||
| Amount: amount, | ||
| }) |
There was a problem hiding this comment.
Normalize and copy amounts in cache writes.
Line 74/81 stores caller-owned *big.Int directly. That allows external alias mutation of cache state and lets nil pointers enter runtime balance state (panic risk downstream during arithmetic/clone).
Suggested fix
func (b InternalBalances) Set(account string, asset string, color string, amount *big.Int) {
+ normalized := new(big.Int)
+ if amount != nil {
+ normalized.Set(amount)
+ }
+
for i := range b[account] {
if b[account][i].Asset == asset && b[account][i].Color == color {
- b[account][i].Amount = amount
+ b[account][i].Amount = normalized
return
}
}
b[account] = append(b[account], AccountBalance{
Asset: asset,
Color: color,
- Amount: amount,
+ Amount: normalized,
})
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/interpreter/internal_balances.go` around lines 71 - 82, In the Set
method of InternalBalances, the code currently stores caller-owned *big.Int
pointers directly (at both the assignment on line 74 and the append on line 81),
which allows external mutation of cached state and permits nil pointers to enter
the runtime balance state. Copy the amount value instead of storing the pointer
directly: create a new big.Int and use the Set method to copy the value (e.g.,
new(big.Int).Set(amount)) at both locations where Amount is assigned, and ensure
nil amounts are handled appropriately by normalizing them to zero or a safe
default big.Int value.
| output = append(output, BalanceRow{ | ||
| Account: row.Account, | ||
| Asset: row.Asset, | ||
| Color: row.Color, | ||
| Amount: new(big.Int).Set(row.Amount), | ||
| }) |
There was a problem hiding this comment.
Guard nil amounts when materializing store results.
Line 51 and Line 62 dereference row.Amount through Set without nil checks. A nil amount row panics the query path.
Suggested fix
func (s StaticStore) GetBalances(_ context.Context, q BalanceQuery) (Balances, error) {
+ cloneAmount := func(v *big.Int) *big.Int {
+ if v == nil {
+ return new(big.Int)
+ }
+ return new(big.Int).Set(v)
+ }
+
var output Balances
for _, item := range q {
baseAsset, isCatchAll := strings.CutSuffix(item.Asset, "/*")
@@
if row.Asset == baseAsset || strings.HasPrefix(row.Asset, baseAsset+"/") {
output = append(output, BalanceRow{
Account: row.Account,
Asset: row.Asset,
Color: row.Color,
- Amount: new(big.Int).Set(row.Amount),
+ Amount: cloneAmount(row.Amount),
})
}
@@
for _, row := range s.Balances {
if row.Account == item.Account && row.Asset == item.Asset && row.Color == item.Color {
- amount.Set(row.Amount)
+ amount = cloneAmount(row.Amount)
break
}
}Also applies to: 61-63
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/interpreter/store.go` around lines 47 - 52, Add nil checks before
dereferencing row.Amount in the BalanceRow initialization within the store
materialization code. When constructing BalanceRow objects and assigning Amount
using new(big.Int).Set(row.Amount), first check if row.Amount is nil to prevent
panics during query execution. Apply this same nil-checking pattern to all
locations where row.Amount is dereferenced through the Set method call, ensuring
the code gracefully handles null amount values.
| err = request.BindArguments(&args) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling for BindArguments failure.
Other errors in this handler (e.g., RequireString at line 53, parse errors at line 62, interpreter errors at line 84) return (mcp.NewToolResultError(...), nil), treating them as tool-level errors. Returning (nil, err) here propagates the error as a server-level error, which may cause different client behavior (e.g., no structured error message).
Proposed fix for consistent error handling
err = request.BindArguments(&args)
if err != nil {
- return nil, err
+ return mcp.NewToolResultError(err.Error()), nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = request.BindArguments(&args) | |
| if err != nil { | |
| return nil, err | |
| } | |
| err = request.BindArguments(&args) | |
| if err != nil { | |
| return mcp.NewToolResultError(err.Error()), nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/mcp_impl/handlers.go` around lines 69 - 72, The error handling for
BindArguments is inconsistent with other error handling in this handler.
Currently, when BindArguments fails, the code returns (nil, err) which
propagates it as a server-level error. Refactor this to wrap the error in
mcp.NewToolResultError(...) and return (mcp.NewToolResultError(...), nil)
instead, matching the pattern used for other errors in the same handler such as
RequireString failures and interpreter errors. This ensures all tool-level
errors are handled consistently as structured tool result errors rather than
server-level errors.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The scoped-account feature is not consistently wired through parsing, analysis, involved-account discovery, and test generation. These issues cause accepted scoped account strings to miss balances, versioned checks to require the wrong flag, the public involved-account API to fail, and generated specs to drop scopes.
| return AccountAddress{}, InvalidAccountName{Name: src} | ||
| } | ||
| return AccountAddress(src), nil | ||
| return AccountAddress{Name: src}, nil |
There was a problem hiding this comment.
🟠 [major] Parse scoped account strings into Scope
When a caller passes an account string that the new regex accepts with a scope suffix, e.g. a variable value src/x, this returns AccountAddress{Name: "src/x", Scope: ""}. Balance queries then look for account="src/x", scope="" and won't match scoped balance rows like {account:"src", scope:"x"}, and postings won't populate sourceScope/destinationScope; split the optional /scope here instead of keeping it in Name.
| VersionConstraints: []VersionClause{ | ||
| { | ||
| Version: parser.NewVersionInterpreter(0, 0, 25), | ||
| FeatureFlag: flags.ExperimentalGetAmountFunctionFeatureFlag, |
There was a problem hiding this comment.
🟠 [major] Gate scoped() with the scoped feature flag
For versioned scripts using scoped(...), the checker currently requires experimental-get-amount-function instead of experimental-scoped-function. This makes numscript check reject scripts that enable the new scoped flag and accept scripts that enable the unrelated get-amount flag, while runtime enforces the scoped flag.
| }, | ||
| }, | ||
| }, | ||
| FnVarOriginScoped: VarOriginFnCallResolution{ |
There was a problem hiding this comment.
🟠 [major] Teach involved-account analysis about scoped()
Registering scoped as an account-returning expression also needs support in GetInvolvedAccounts: scripts that use scoped(@src, "x") in a source, destination, balance, or metadata expression now reach the involved-account evaluator's function switch and fall through to UnboundFunctionErr. This breaks the public ParseResult.GetInvolvedAccounts API for the new feature.
| } | ||
| b.Amount = amount | ||
|
|
||
| s.StaticStore.Balances = append(s.StaticStore.Balances, interpreter.BalanceRow{ |
There was a problem hiding this comment.
🟠 [major] Preserve scope in test-init generated balances
When test-init funds an unknown scoped balance, the returned query row has b.Scope set, but the row saved into s.StaticStore.Balances omits it. Generated specs for scripts such as scoped(@src, "x") therefore contain an unscoped initial balance, so rerunning the generated spec queries the scoped balance as zero and produces incorrect expected balances/failures.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/specs_format/index.go (1)
274-289:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
validateSpecsis defined but never called.The
validateSpecsfunction validates that balance lists don't contain duplicate entries, butCheck()never invokes it. Duplicate balance entries will silently use the last value instead of being rejected as the docstring intends.Add
if err := validateSpecs(specs); err != nil { return SpecsResult{}, ... }at the start ofCheck().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specs_format/index.go` around lines 274 - 289, The validateSpecs function is defined but never invoked, allowing duplicate balance entries to silently use the last value instead of being rejected. Add a call to validateSpecs at the beginning of the Check() function to validate the specs Balances before any processing occurs; if validateSpecs returns an error, return a SpecsResult with the error rather than continuing.
♻️ Duplicate comments (2)
internal/interpreter/value.go (1)
44-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScoped account strings passed as variable values are not parsed into separate Name/Scope.
When a caller passes an account string containing a scope suffix (e.g.,
"src/x"as a variable value),NewAccountAddressstores it entirely in theNamefield without extracting the scope. This meansAccountAddress{Name: "src/x", Scope: ""}is created instead of{Name: "src", Scope: "x"}.Balance queries and postings will then use the wrong keys, looking for
account="src/x", scope=""rather thanaccount="src", scope="x".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/interpreter/value.go` around lines 44 - 49, The NewAccountAddress function does not parse scoped account strings that contain a scope suffix. When a string like "src/x" is passed as the src parameter, the entire string is stored in the Name field without extracting the scope portion. Modify NewAccountAddress to parse the src string, extract any scope suffix (typically separated by a delimiter), and populate both the Name and Scope fields of the returned AccountAddress struct accordingly. The validation with checkAccountName should be applied appropriately to handle both scoped and unscoped account strings.internal/interpreter/interpreter.go (1)
199-206:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAccount regex rejects digit-containing scopes that
validateScopeaccepts.The regex pattern
(?:/[a-z_]+)?only allows lowercase letters and underscores in the scope suffix, butvalidateScopeaccepts digits (e.g.,"x1"). A script usingscoped(@src, "x1")will pass scope validation during interpretation but then failcheckPostingInvariantswhen the posting is created.Align the regex with
validateScopeby changing[a-z_]+to[a-z0-9_]+.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/interpreter/interpreter.go` around lines 199 - 206, The account name regex pattern in accountNameRegex uses a character class [a-z_]+ for the scope suffix that does not match the validation logic in validateScope, which accepts digits. Update the regex pattern to include digits by changing the scope suffix pattern from (?:/[a-z_]+)? to (?:/[a-z0-9_]+)? so that the regex accepts digit-containing scopes like "x1" that validateScope permits, preventing validation failures downstream.
🧹 Nitpick comments (3)
internal/interpreter/value_test.go (1)
43-51: ⚡ Quick winConsider adding test coverage for scoped address JSON marshaling.
The test only covers an
AccountAddresswithout a scope. Adding a test case forAccountAddress{Name: "abc", Scope: "xyz"}marshaling to"abc/xyz"would ensure the scope formatting logic is verified.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/interpreter/value_test.go` around lines 43 - 51, The TestMarshalAddress function currently only tests marshaling of AccountAddress with a Name field but no Scope. Add an additional test case within TestMarshalAddress to verify that AccountAddress with both Name and Scope fields (e.g., Name: "abc", Scope: "xyz") marshals correctly to the format "abc/xyz". This ensures the scope formatting logic is properly covered by the test suite.internal/interpreter/interpreter.go (1)
581-586: 💤 Low valueDirect map access on
CachedBalancesassumes account was pre-queried.The code accesses
s.CachedBalances[account]directly and returns an error if the key is missing. This relies on the balance query batching having populated that key. If the batching logic ever changes or misses a path, this will surface as an opaqueInvalidUnboundedAddressInScalingAddresserror rather than indicating the actual cache miss.This is a potential fragility but not an immediate bug given current batching coverage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/interpreter/interpreter.go` around lines 581 - 586, The direct map access on s.CachedBalances[account] at the point where the ok check fails assumes the balance query batching has already populated that key, but provides no explicit verification or clear indication when a cache miss occurs. Instead of returning the generic InvalidUnboundedAddressInScalingAddress error, add logging or improve the error message to explicitly indicate that the account's balance was not found in the cache, making it clear that the issue is a cache miss rather than an unbounded address problem. This will help surface any gaps in the balance query batching logic more explicitly.internal/interpreter/value.go (1)
275-287: 💤 Low valueSilent fallback to scale=0 on parse failure may hide malformed assets.
When the scale suffix cannot be parsed as an integer,
GetBaseAndScalesilently returnsscale=0. This could mask malformed asset strings that look valid but have non-numeric scale suffixes (e.g.,"USD/abc").Consider whether callers need to distinguish between "no scale specified" and "invalid scale format".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/interpreter/value.go` around lines 275 - 287, The GetBaseAndScale function on the Asset type silently masks malformed asset strings by returning scale=0 when the scale suffix fails to parse as an integer, making it impossible for callers to distinguish between a missing scale and an invalid scale format. Modify the GetBaseAndScale function signature to return a third error value (changing from (string, int64) to (string, int64, error)), and when the strconv.ParseInt call fails, return the error instead of silently returning 0. This allows callers to detect and handle genuinely malformed assets like "USD/abc" rather than treating them the same as valid assets without a scale suffix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/specs_format/index.go`:
- Around line 274-289: The validateSpecs function is defined but never invoked,
allowing duplicate balance entries to silently use the last value instead of
being rejected. Add a call to validateSpecs at the beginning of the Check()
function to validate the specs Balances before any processing occurs; if
validateSpecs returns an error, return a SpecsResult with the error rather than
continuing.
---
Duplicate comments:
In `@internal/interpreter/interpreter.go`:
- Around line 199-206: The account name regex pattern in accountNameRegex uses a
character class [a-z_]+ for the scope suffix that does not match the validation
logic in validateScope, which accepts digits. Update the regex pattern to
include digits by changing the scope suffix pattern from (?:/[a-z_]+)? to
(?:/[a-z0-9_]+)? so that the regex accepts digit-containing scopes like "x1"
that validateScope permits, preventing validation failures downstream.
In `@internal/interpreter/value.go`:
- Around line 44-49: The NewAccountAddress function does not parse scoped
account strings that contain a scope suffix. When a string like "src/x" is
passed as the src parameter, the entire string is stored in the Name field
without extracting the scope portion. Modify NewAccountAddress to parse the src
string, extract any scope suffix (typically separated by a delimiter), and
populate both the Name and Scope fields of the returned AccountAddress struct
accordingly. The validation with checkAccountName should be applied
appropriately to handle both scoped and unscoped account strings.
---
Nitpick comments:
In `@internal/interpreter/interpreter.go`:
- Around line 581-586: The direct map access on s.CachedBalances[account] at the
point where the ok check fails assumes the balance query batching has already
populated that key, but provides no explicit verification or clear indication
when a cache miss occurs. Instead of returning the generic
InvalidUnboundedAddressInScalingAddress error, add logging or improve the error
message to explicitly indicate that the account's balance was not found in the
cache, making it clear that the issue is a cache miss rather than an unbounded
address problem. This will help surface any gaps in the balance query batching
logic more explicitly.
In `@internal/interpreter/value_test.go`:
- Around line 43-51: The TestMarshalAddress function currently only tests
marshaling of AccountAddress with a Name field but no Scope. Add an additional
test case within TestMarshalAddress to verify that AccountAddress with both Name
and Scope fields (e.g., Name: "abc", Scope: "xyz") marshals correctly to the
format "abc/xyz". This ensures the scope formatting logic is properly covered by
the test suite.
In `@internal/interpreter/value.go`:
- Around line 275-287: The GetBaseAndScale function on the Asset type silently
masks malformed asset strings by returning scale=0 when the scale suffix fails
to parse as an integer, making it impossible for callers to distinguish between
a missing scale and an invalid scale format. Modify the GetBaseAndScale function
signature to return a third error value (changing from (string, int64) to
(string, int64, error)), and when the strconv.ParseInt call fails, return the
error instead of silently returning 0. This allows callers to detect and handle
genuinely malformed assets like "USD/abc" rather than treating them the same as
valid assets without a scale suffix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b25e2ab-7af6-44c2-882c-bbc9c4753249
📒 Files selected for processing (18)
internal/interpreter/append_scope.gointernal/interpreter/append_scope_test.gointernal/interpreter/args_parser_test.gointernal/interpreter/balances.gointernal/interpreter/batch_balances_query.gointernal/interpreter/function_exprs.gointernal/interpreter/function_statements.gointernal/interpreter/funds_queue.gointernal/interpreter/funds_queue_test.gointernal/interpreter/get_involved_accounts.gointernal/interpreter/internal_balances.gointernal/interpreter/internal_balances_test.gointernal/interpreter/interpreter.gointernal/interpreter/interpreter_test.gointernal/interpreter/store.gointernal/interpreter/value.gointernal/interpreter/value_test.gointernal/specs_format/index.go
💤 Files with no reviewable changes (1)
- internal/interpreter/append_scope.go
✅ Files skipped from review due to trivial changes (1)
- internal/interpreter/internal_balances_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/interpreter/balances.go
- internal/interpreter/function_exprs.go
- internal/interpreter/args_parser_test.go
- internal/interpreter/interpreter_test.go
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The patch wires the new scoped function to the wrong static-analysis feature flag and introduces account/metadata encoding mismatches that can reject valid scoped use cases or emit invalid account identifiers.
| VersionConstraints: []VersionClause{ | ||
| { | ||
| Version: parser.NewVersionInterpreter(0, 0, 25), | ||
| FeatureFlag: flags.ExperimentalGetAmountFunctionFeatureFlag, |
There was a problem hiding this comment.
🟠 [major] Use the scoped feature flag for scoped()
For versioned scripts, scoped() is currently gated by experimental-get-amount-function instead of experimental-scoped-function. A script that enables the documented scoped flag will still get an experimental-feature diagnostic for get_amount, while enabling get_amount can make static analysis accept a script that the runtime later rejects because scoped() checks the scoped flag.
| const accountSegmentRegex = "[a-zA-Z0-9_-]+" | ||
|
|
||
| var accountNameRegex = regexp.MustCompile("^" + accountSegmentRegex + "(:" + accountSegmentRegex + ")*$") | ||
| var accountNameRegex = regexp.MustCompile("^@?" + accountSegmentRegex + "(:" + accountSegmentRegex + ")*(?:/[a-z_]+)?$") |
There was a problem hiding this comment.
🟠 [major] Reject encoded syntax in bare account names
For bound account variables, this validates the raw account name, not a Numscript literal, and scopes are modeled in AccountAddress.Scope; accepting values like @alice or alice/x stores those characters in Name instead of rejecting or parsing them. Those values then produce balance queries/postings with invalid bare account names that cannot match schema-valid scoped balance rows such as {account:"alice", scope:"x"}.
| @@ -62,19 +62,19 @@ func meta( | |||
| } | |||
|
|
|||
| meta, fetchMetaErr := s.Store.GetAccountsMetadata(s.ctx, MetadataQuery{ | |||
There was a problem hiding this comment.
🟠 [major] Make scoped metadata representable in JSON schemas
When meta(scoped(@a, "x"), "k") runs, this now queries metadata under the key a/x, and set_account_meta uses the same encoded key. The input/spec metadata schemas still reject / in account metadata keys, so schema-valid JSON cannot provide or assert metadata for scoped accounts even though the interpreter now looks it up there.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #161 +/- ##
==========================================
+ Coverage 67.81% 68.09% +0.28%
==========================================
Files 49 51 +2
Lines 5223 5316 +93
==========================================
+ Hits 3542 3620 +78
- Misses 1472 1478 +6
- Partials 209 218 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The patch introduces a public type alias to an undefined interpreter type, so the package cannot build as-is. It also wires the new scoped builtin to the wrong experimental flag.
| BalanceRow = interpreter.BalanceRow | ||
|
|
||
| AccountMetadata = interpreter.AccountMetadata | ||
| AccountMetadataRow = interpreter.AccountMetadataRow |
There was a problem hiding this comment.
🔴 [blocker] Define the metadata row type before aliasing it
Any build of the public package now fails because this alias refers to interpreter.AccountMetadataRow, but no such type exists in internal/interpreter (the metadata model is still AccountsMetadata map[string]AccountMetadata). This also leaves the new array-shaped metadata literals/schema out of sync with the Go types.
| Docs: "returns the scoped version of that account. Empty string means no scope. Overwrites the previous scope", | ||
| VersionConstraints: []VersionClause{ | ||
| { | ||
| Version: parser.NewVersionInterpreter(0, 0, 25), |
There was a problem hiding this comment.
🟠 [major] Require the scoped feature flag for scoped()
When a script uses scoped() with experimental-scoped-function enabled, the analyzer still reports the missing experimental-get-amount-function flag because the builtin is registered with the get-amount flag here. This makes the documented/tested scoped flag insufficient during version-gated analysis.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@numscript.go`:
- Line 63: The AccountMetadataRow alias on line 63 points to
interpreter.AccountMetadataRow which is not defined or exported, causing a build
failure. Either locate the correct exported type name from the interpreter
package and update the alias to reference that existing type, or ensure that
AccountMetadataRow is properly exported from the internal/interpreter package so
it can be aliased here.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 14d3feb5-8866-4448-9cf0-e92ffcf4d5b8
⛔ Files ignored due to path filters (6)
inputs.schema.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/metadata.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/neg-max-dest.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/override-account-meta.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/set-account-meta.num.specs.jsonis excluded by!**/*.jsonspecs.schema.jsonis excluded by!**/*.json
📒 Files selected for processing (6)
internal/cmd/test_init.gointernal/interpreter/function_exprs.gointernal/interpreter/interpreter_test.gointernal/interpreter/store.gonumscript.gonumscript_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/interpreter/store.go
- internal/interpreter/interpreter_test.go
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The scoped function is registered with the wrong analysis feature flag, causing static checks and runtime behavior to disagree for the new feature.
| VersionConstraints: []VersionClause{ | ||
| { | ||
| Version: parser.NewVersionInterpreter(0, 0, 25), | ||
| FeatureFlag: flags.ExperimentalGetAmountFunctionFeatureFlag, |
There was a problem hiding this comment.
🟠 [major] Use the scoped feature flag for scoped()
When a script is checked by the analysis layer, scoped() is gated by experimental-get-amount-function here, while the interpreter itself checks experimental-scoped-function. This means a script that enables the new scoped flag still gets an analysis diagnostic for a missing get-amount flag, and a script with only the get-amount flag can pass analysis but fail at runtime.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/interpreter/accounts_metadata.go (1)
22-30: ⚡ Quick winInclude
Scope/Colorin metadata pretty-print output.Line 27 currently drops scoped/color dimensions, so distinct metadata rows can print identically and become hard to debug.
Suggested refactor
func (m AccountsMetadata) PrettyPrint() string { - header := []string{"Account", "Name", "Value"} + header := []string{"Account", "Name", "Value"} + showColor := false + showScope := false + for _, row := range m { + showColor = showColor || row.Color != "" + showScope = showScope || row.Scope != "" + } + if showColor { + header = append(header, "Color") + } + if showScope { + header = append(header, "Scope") + } var rows [][]string for _, row := range m { - rows = append(rows, []string{row.Account, row.Key, row.Value}) + out := []string{row.Account, row.Key, row.Value} + if showColor { + out = append(out, row.Color) + } + if showScope { + out = append(out, row.Scope) + } + rows = append(rows, out) } return utils.CsvPretty(header, rows, true) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/interpreter/accounts_metadata.go` around lines 22 - 30, The PrettyPrint method in AccountsMetadata is missing the Scope and Color dimensions in its output, which causes distinct metadata rows to print identically. Add "Scope" and "Color" to the header slice alongside "Account", "Name", and "Value", and then modify the loop that builds rows to include the Scope and Color fields from each row object (row.Scope and row.Color) when appending to the rows slice, ensuring all dimensions are captured in the formatted output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/interpreter/accounts_metadata.go`:
- Around line 35-44: The CompareAccountsMetadata function uses slices.Contains
to check if elements from slice a exist in slice b, but this approach only
verifies presence and not count, allowing unequal datasets with duplicate values
to incorrectly return true. Fix this by implementing proper multiset comparison
that accounts for element counts: either sort both AccountsMetadata slices and
use slices.Equal for direct comparison, or use a map-based approach to count
occurrences of each element in both slices and verify the counts match. Ensure
the fix correctly handles the case where slices have equal length but different
element frequencies.
---
Nitpick comments:
In `@internal/interpreter/accounts_metadata.go`:
- Around line 22-30: The PrettyPrint method in AccountsMetadata is missing the
Scope and Color dimensions in its output, which causes distinct metadata rows to
print identically. Add "Scope" and "Color" to the header slice alongside
"Account", "Name", and "Value", and then modify the loop that builds rows to
include the Scope and Color fields from each row object (row.Scope and
row.Color) when appending to the rows slice, ensuring all dimensions are
captured in the formatted output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: af3f1cfb-a4a2-4cfc-853a-d9e730c2b869
📥 Commits
Reviewing files that changed from the base of the PR and between 7647b9b and d353ea38728c6328e982063733274bc53bdd25c1.
📒 Files selected for processing (6)
internal/interpreter/accounts_metadata.gointernal/interpreter/function_exprs.gointernal/interpreter/function_statements.gointernal/interpreter/internal_accounts_metadata.gointernal/interpreter/interpreter.gointernal/specs_format/index.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/interpreter/function_exprs.go
- internal/specs_format/index.go
- internal/interpreter/interpreter.go
| func CompareAccountsMetadata(a AccountsMetadata, b AccountsMetadata) bool { | ||
| if len(a) != len(b) { | ||
| return false | ||
| } | ||
| for _, row := range a { | ||
| if !slices.Contains(b, row) { | ||
| return false | ||
| } | ||
| } | ||
| return true |
There was a problem hiding this comment.
Fix multiset comparison in CompareAccountsMetadata.
This implementation can return true for unequal datasets when duplicates exist (e.g., Line 39 loop + slices.Contains only checks presence, not count). That can mask assertion failures where this comparator is used by specs.
Suggested fix
-import (
- "slices"
-
- "github.com/formancehq/numscript/internal/utils"
-)
+import "github.com/formancehq/numscript/internal/utils"
@@
func CompareAccountsMetadata(a AccountsMetadata, b AccountsMetadata) bool {
if len(a) != len(b) {
return false
}
- for _, row := range a {
- if !slices.Contains(b, row) {
+ counts := make(map[AccountMetadataRow]int, len(a))
+ for _, row := range a {
+ counts[row]++
+ }
+ for _, row := range b {
+ if counts[row] == 0 {
return false
}
+ counts[row]--
}
return true
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func CompareAccountsMetadata(a AccountsMetadata, b AccountsMetadata) bool { | |
| if len(a) != len(b) { | |
| return false | |
| } | |
| for _, row := range a { | |
| if !slices.Contains(b, row) { | |
| return false | |
| } | |
| } | |
| return true | |
| func CompareAccountsMetadata(a AccountsMetadata, b AccountsMetadata) bool { | |
| if len(a) != len(b) { | |
| return false | |
| } | |
| counts := make(map[AccountMetadataRow]int, len(a)) | |
| for _, row := range a { | |
| counts[row]++ | |
| } | |
| for _, row := range b { | |
| if counts[row] == 0 { | |
| return false | |
| } | |
| counts[row]-- | |
| } | |
| return true | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/interpreter/accounts_metadata.go` around lines 35 - 44, The
CompareAccountsMetadata function uses slices.Contains to check if elements from
slice a exist in slice b, but this approach only verifies presence and not
count, allowing unequal datasets with duplicate values to incorrectly return
true. Fix this by implementing proper multiset comparison that accounts for
element counts: either sort both AccountsMetadata slices and use slices.Equal
for direct comparison, or use a map-based approach to count occurrences of each
element in both slices and verify the counts match. Ensure the fix correctly
handles the case where slices have equal length but different element
frequencies.
d353ea3 to
62c70e1
Compare
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The runtime support is incomplete around tooling/public APIs: static validation checks the wrong feature flag, involved-account analysis rejects scoped scripts, and test initialization drops the new scope dimension when funding balances.
| VersionConstraints: []VersionClause{ | ||
| { | ||
| Version: parser.NewVersionInterpreter(0, 0, 25), | ||
| FeatureFlag: flags.ExperimentalGetAmountFunctionFeatureFlag, |
There was a problem hiding this comment.
🟠 [major] Require the scoped feature flag for scoped()
For versioned scripts that use scoped() with experimental-scoped-function enabled, the checker/LSP will still report that experimental-get-amount-function is missing because this constraint points at the get_amount flag. This makes the new feature fail static validation unless callers enable an unrelated flag, while the interpreter itself checks ExperimentalScopedFunction.
| @@ -114,6 +120,17 @@ var Builtins = map[string]FnCallResolution{ | |||
| }, | |||
| }, | |||
| }, | |||
| FnVarOriginScoped: VarOriginFnCallResolution{ | |||
There was a problem hiding this comment.
🟠 [major] Handle scoped() in involved-account analysis
Adding scoped() as a supported account expression leaves GetInvolvedAccounts incomplete: its function-call switch still has no FnVarOriginScoped case, so any script using scoped(@acc, "x") in a source/destination is executable but the public involved-accounts API returns UnboundFunctionErr. Please add an involved-account expression for scoped accounts or fold it into the account expression there as well.
| @@ -11,12 +11,18 @@ type BalanceQueryItem struct { | |||
| Account string | |||
| Asset string | |||
| Color string | |||
| Scope string | |||
There was a problem hiding this comment.
🟠 [major] Propagate scope in test-init balance funding
Once balance queries include Scope, TestInitStore.GetBalances also needs to key and append funded balances with that scope. As-is, numscript test init treats acc/x and acc/y as the same (account, asset, color) key and writes generated specs without the queried scope, so scoped scripts can get only one scoped source funded and the generated specs won't replay correctly.
Model accounts as a struct carrying an optional scope instead of encoding
it into the address string. Scope flows structured through balances,
postings, the funds queue, balance/metadata queries and the store.
Account metadata is now row-based externally (AccountsMetadata as a list of
AccountMetadataRow, mirroring Balances) and indexed by a {account, scope,
key} map internally (InternalAccountsMetadata) for fast lookups.
ascandone
force-pushed
the
feat/scoped-accounts
branch
from
62c70e1 to
c07d256
Compare
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The scoped builtin is not consistently integrated: static analysis requires the wrong feature flag, and involved-account discovery errors on scripts using the new function. These are functional regressions for supported tooling/API paths.
| VersionConstraints: []VersionClause{ | ||
| { | ||
| Version: parser.NewVersionInterpreter(0, 0, 25), | ||
| FeatureFlag: flags.ExperimentalGetAmountFunctionFeatureFlag, |
There was a problem hiding this comment.
🟠 [major] Use the scoped feature flag for scoped()
For versioned scripts that enable experimental-scoped-function, the analyzer still requires experimental-get-amount-function because this constraint is wired to the wrong flag. This makes scoped(...) report the wrong experimental-feature diagnostic even though the interpreter checks ExperimentalScopedFunction at runtime.
| return getAsset(s, fnCall.Range, args) | ||
| case analysis.FnVarOriginGetAmount: | ||
| return getAmount(s, fnCall.Range, args) | ||
| case analysis.FnVarOriginScoped: |
There was a problem hiding this comment.
🟠 [major] Teach involved-account discovery about scoped()
When callers use ParseResult.GetInvolvedAccounts on a script that uses scoped(...) in a source, destination, balance, or meta account expression, the discovery evaluator has no FnVarOriginScoped case and returns UnboundFunctionErr. Adding the runtime handler here makes the script executable, but the public preflight/discovery API still cannot process the new builtin.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The scoped-account implementation is incomplete: analysis gates the new builtin on the wrong feature flag, raw scoped account strings are accepted without mapping to the new scope dimension, and the public involved-account analysis cannot process the new builtin.
| VersionConstraints: []VersionClause{ | ||
| { | ||
| Version: parser.NewVersionInterpreter(0, 0, 25), | ||
| FeatureFlag: flags.ExperimentalGetAmountFunctionFeatureFlag, |
There was a problem hiding this comment.
🟠 [major] Use the scoped feature flag for scoped()
When a versioned script enables experimental-scoped-function, numscript check still reports that experimental-get-amount-function is required, while enabling only get-amount can pass analysis and then fail at runtime because scoped() checks ExperimentalScopedFunction. This constraint should reference the same scoped flag used by the interpreter.
| const accountSegmentRegex = "[a-zA-Z0-9_-]+" | ||
|
|
||
| var accountNameRegex = regexp.MustCompile("^" + accountSegmentRegex + "(:" + accountSegmentRegex + ")*$") | ||
| var accountNameRegex = regexp.MustCompile("^@?" + accountSegmentRegex + "(:" + accountSegmentRegex + ")*(?:/[a-z_]+)?$") |
There was a problem hiding this comment.
🟠 [major] Normalize or reject encoded scoped account names
This now accepts account variable values such as alice/eu (and @alice), but NewAccountAddress stores the entire string in Name with an empty Scope. A send from variable alice/eu will query balances/metadata as account alice/eu, scope `` rather than matching the new {account:"alice", scope:"eu"} representation, and `@alice` can be emitted as a posting account even though `@` is only syntax. These forms need to be split/stripped here or kept invalid.
| return mon.Amount, nil | ||
| } | ||
|
|
||
| func scoped( |
There was a problem hiding this comment.
🟠 [major] Handle scoped() in involved-account analysis
Scripts using the new scoped() builtin run through the interpreter, but ParseResult.GetInvolvedAccounts still evaluates function calls with a switch that has no scoped case, so a source or destination like scoped(@src, "x") returns UnboundFunctionErr. Since this PR adds scoped() as an account expression, the involved-account evaluator/model needs a matching case.
| Asset string `json:"asset"` | ||
| Amount *big.Int `json:"amount"` | ||
| Color string `json:"color,omitempty"` | ||
| Scope string `json:"scope,omitempty"` |
There was a problem hiding this comment.
🟡 [minor] Show scope when printing balances
Once balances can differ by Scope, Balances.PrettyPrint still renders only account/asset/color/amount, so two rows that differ only by scope are displayed as indistinguishable balances in pretty output. The balance pretty-printer should include a Scope column when any row has a scope, like account metadata does.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The scoped-account changes leave several paths inconsistent with the new scope dimension, including raw account parsing, analyzer feature gating, and generated test specs. These issues can reject valid scoped scripts or produce balances/postings that do not match scoped inputs.
| const accountSegmentRegex = "[a-zA-Z0-9_-]+" | ||
|
|
||
| var accountNameRegex = regexp.MustCompile("^" + accountSegmentRegex + "(:" + accountSegmentRegex + ")*$") | ||
| var accountNameRegex = regexp.MustCompile("^@?" + accountSegmentRegex + "(:" + accountSegmentRegex + ")*(?:/[a-z_]+)?$") |
There was a problem hiding this comment.
🟠 [major] Normalize accepted account prefixes and scopes
When raw account values come from variables, this regex now accepts values like @alice and alice/s1, but NewAccountAddress stores the entire string in Name and leaves Scope empty. Those accepted inputs will query/post @alice or alice/s1 verbatim instead of alice with scope s1, so they won't match scoped balance rows and may emit account names outside the new posting contract.
| Docs: "returns the scoped version of that account. Empty string means no scope. Overwrites the previous scope", | ||
| VersionConstraints: []VersionClause{ | ||
| { | ||
| Version: parser.NewVersionInterpreter(0, 0, 25), |
There was a problem hiding this comment.
🟠 [major] Use the scoped feature flag for scoped()
For versioned scripts that call scoped, the analyzer currently requires experimental-get-amount-function instead of experimental-scoped-function. A script declaring the scoped flag will still get an experimental-feature diagnostic, while declaring the get-amount flag lets analysis pass but runtime rejects the call unless the scoped flag is also enabled.
| Asset string `json:"asset"` | ||
| Amount *big.Int `json:"amount"` | ||
| Color string `json:"color,omitempty"` | ||
| Scope string `json:"scope,omitempty"` |
There was a problem hiding this comment.
🟠 [major] Preserve scope when generated specs seed balances
Adding Scope makes it part of a balance's identity, but TestInitStore.GetBalances still tracks and appends generated balances only by account/asset/color. When test-init funds a scoped source, it records an unscoped balance in the generated specs, and a second scope for the same account can be treated as already funded and left at zero.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The scoped-account feature is not consistently wired through analysis, public involved-account discovery, account parsing, and test-init balance materialization. These issues cause valid scoped scripts or generated specs to fail in common paths.
| VersionConstraints: []VersionClause{ | ||
| { | ||
| Version: parser.NewVersionInterpreter(0, 0, 25), | ||
| FeatureFlag: flags.ExperimentalGetAmountFunctionFeatureFlag, |
There was a problem hiding this comment.
🟠 [major] Gate scoped() on the scoped feature flag
For versioned scripts that enable @feature_flag experimental-scoped-function, analysis still reports that experimental-get-amount-function is required because the new scoped builtin is wired to the wrong feature flag here. This makes CLI/LSP checks reject the intended flag and accept the wrong one, while runtime checks ExperimentalScopedFunction.
| return getAsset(s, fnCall.Range, args) | ||
| case analysis.FnVarOriginGetAmount: | ||
| return getAmount(s, fnCall.Range, args) | ||
| case analysis.FnVarOriginScoped: |
There was a problem hiding this comment.
🟠 [major] Handle scoped() in involved-account analysis
Scripts that use scoped(...) in a source, destination, or account metadata argument now run, but ParseResult.GetInvolvedAccounts still has no FnVarOriginScoped case in its expression evaluator, so those same scripts return UnboundFunctionErr from that public API. The new builtin needs a corresponding involved-account expression case so callers can precompute scoped accounts.
| Account string | ||
| Asset string | ||
| Color string | ||
| Scope string |
There was a problem hiding this comment.
🟠 [major] Preserve query scopes in test-init balances
Once balance queries include Scope, TestInitStore.GetBalances also needs to key stored balances by scope and append BalanceRow{Scope: b.Scope}. With numscript test-init on a script using scoped(@src, "x"), the generated specs currently record an unscoped default balance, so replaying the generated spec cannot fund the scoped source.
| const accountSegmentRegex = "[a-zA-Z0-9_-]+" | ||
|
|
||
| var accountNameRegex = regexp.MustCompile("^" + accountSegmentRegex + "(:" + accountSegmentRegex + ")*$") | ||
| var accountNameRegex = regexp.MustCompile("^@?" + accountSegmentRegex + "(:" + accountSegmentRegex + ")*(?:/[a-z_]+)?$") |
There was a problem hiding this comment.
🟠 [major] Parse accepted slash scopes into AccountAddress.Scope
When an account variable is supplied in the slash form produced by AccountAddress.String() such as src/x, this regex now accepts it, but NewAccountAddress stores the whole string in Name and leaves Scope empty. Balance and metadata queries then ask for account src/x with no scope instead of account src with scope x, so they cannot match rows using the new scope field.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The scoped feature flag is wired to the wrong analysis flag, and slash-scoped account strings are accepted without being converted into the new structured scope field. These cases break valid scoped-account usage.
| VersionConstraints: []VersionClause{ | ||
| { | ||
| Version: parser.NewVersionInterpreter(0, 0, 25), | ||
| FeatureFlag: flags.ExperimentalScopedFunction, |
There was a problem hiding this comment.
🟠 [major] Use the scoped feature flag for scoped()
For versioned programs using scoped(), the static checker currently requires experimental-get-amount-function instead of experimental-scoped-function. A script that enables the new scoped flag will still get an experimental-feature diagnostic for the wrong flag, while enabling the get-amount flag passes analysis but then fails at runtime because scoped() checks the scoped flag.
| return AccountAddress{}, InvalidAccountName{Name: src} | ||
| } | ||
| return AccountAddress(src), nil | ||
| return AccountAddress{Name: src}, nil |
There was a problem hiding this comment.
🟠 [major] Parse slash-scoped account strings into Scope
When an account is supplied as a raw variable/API value in the same string form emitted by AccountAddress.String(), e.g. src/x, this constructor now accepts it but stores the whole value in Name with an empty Scope. Balance and metadata lookups then query account="src/x", scope="" instead of matching rows like account="src", scope="x", so scoped account variables cannot use the new scoped balances/metadata representation.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The scoped-account representation is not normalized when account strings are parsed, and the validator now accepts invalid @-prefixed runtime account names. These cases can produce incorrect balance queries/postings for accepted inputs.
| return AccountAddress{}, InvalidAccountName{Name: src} | ||
| } | ||
| return AccountAddress(src), nil | ||
| return AccountAddress{Name: src}, nil |
There was a problem hiding this comment.
🟠 [major] Parse scoped account strings into the Scope field
When an account value comes from variables, metadata, or interpolation as acc/x, the new regex accepts it but this constructor stores the whole string in Name with an empty Scope. Balance queries and postings then use account acc/x rather than (account: acc, scope: x), so they won't match scoped balances/metadata and won't populate sourceScope/destinationScope. Either split the slash form here or keep rejecting it.
| const accountSegmentRegex = "[a-zA-Z0-9_-]+" | ||
|
|
||
| var accountNameRegex = regexp.MustCompile("^" + accountSegmentRegex + "(:" + accountSegmentRegex + ")*$") | ||
| var accountNameRegex = regexp.MustCompile("^@?" + accountSegmentRegex + "(:" + accountSegmentRegex + ")*(?:/[a-z_]+)?$") |
There was a problem hiding this comment.
🟠 [major] Reject @ prefixes in runtime account values
Because this validation is also used for account variables and metadata-derived account values, the new optional @ accepts inputs like @alice. That marker is DSL syntax and is never stripped, so subsequent balance queries and postings are emitted for account @alice, which does not match balances keyed as alice and violates the account/schema format.
2 participants
No description provided.